Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

자동차경주/제훈/step1 #4

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

JoJeHuni
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@jonghyunhub jonghyunhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미션 고생하셨습니다~ 열심히 달려봅시다 💪


public class Car {

// public static void main(String[] args) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필요없는 주석은 삭제해주세요~

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Car 객체에서 main함수를 만들고 사용하려고 생성자를 private으로 만드신것 같네요~

Car 객체가

  1. main 함수도 실행하고
  2. 도메인 로직도 실행하고
  3. 입력도 받고
  4. 출력도 하고

많은 것을 하고 있네요.

Car 객체가 맡은 책임이 너무 많은것이 아닐까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구조를 이렇게 만들어보면 어떨까요?

main 함수를 가지고 있고 전체 프로그램을 실행시키는 : RacingCarApplication

View --------------------------------
ui 로직중 입력을 담당하는 : InputView
ui 로직중 출력을 담당하는 : ResultView

Controller ----------------------------
도메인 객체들에게 필요한 로직을 요청하는 오케스트레이션 객체 : CarController

model -------------------------------
도메인 객체들 : Car, .. 등등

물론 이 구조가 정답은 아니겠지만 이런식으로 나눠서 만들어보면 객체들의 책임을 나눠서 만들기 편하더라구요~

https://m.blog.naver.com/jhc9639/220967034588
Mvc 패턴에 대해 한번 공부해보시는게 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 패키지를 View, controller, model로 만들어서 객체들을 관리하는 것도 좋을 것 같아요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MVC패턴 강의 영상을 여러 번 곱씹어 봐야겠네요 너무 도움되는 조언 감사합니다 참고해서 바로 수정해보겠습니다!

}
public static void numberOfCar() {
Scanner scanner = new Scanner(System.in);
System.out.println("자동차 대수는 몇 대 인가요?");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프로그래밍 요구사항을 지켜보아요!

프로그래밍 요구 사항

  • 핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.
  • UI 로직을 InputView, ResultView와 같은 클래스를 추가해 분리한다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

도메인 객체와 입출력을 담당하는 객체는 분리하는 것이 좋아요.

왜 그럴까요?
우리가 도메인 객체에게 기대하는 것은 무엇일까요?
"만들고자 하는 비즈니스 로직대로 동작하는 것"이 아닐까요?

도메인 객체의 입장에서는 입/출력은 중요한것이 아닌것이 아닐까요?
그렇게 생각해보면 "도메인 객체의 입장에서 중요하지 않은" 입/출력 로직을 다른 객체한테 위임하고
중요한 "비즈니스 로직대로 동작하는 것"에 집중하면 되는게 아닐까요?

객체가 한가지 책임에 집중해야 결과물이 더 좋다고 할 수 있겠죠?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 책임을 나눠야 하는 이유에 대해서 이야기해보면
객체가 많은 책임을 맡고 있다는 것은
다르게 이야기하면 "변경해야 할 이유가 많다는 것"이에요.

이게 무슨 이야기일까요?

다시 한번 Car 객체가 하고 있는 것들을 나열해볼게요.

Car 객체가 하는 일 목록

  1. main 함수도 실행
  2. 도메인 로직도 실행
  3. 입력도 받고
  4. 출력도 하고

네가지의 일을 하고 있네요.

자 그러면 만약에 프로그래밍 요구 사항이 변경되어서

입력을 받는 일에 변경이 생겼다고 합시다.

그러면 어디를 변경해야 할까요?
Car 객체를 변경해야 겠지요?

혹은 출력과 관련된 요구사항이 바뀌면 어떻죠?
이번에도 Car 객체를 변경해야 겠네요?

그리고 제일 극악무도한 사실은 도메인 로직이네요!
소프트웨어는 생명체와 같아서 변경은 밥먹듯이 일어납니다.
그중 도메인 로직은 가장 변경이 많이 생기는 지점이기도 하지요.

그러면 Car 객체는 무수한 도메인 로직의 변경사항을 반영해서 변경이 무수한 변경이 일어나겠네요!

혹여나 협업으로 Car 클래스를 관리한다고 하면 상황은 더 끔찍합니다...

이런 맥락에서 객체의 책임을 나눠주어야 한답니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

도메인 객체의 입장에서 중요하지 않는 로직은 다른 객체에 위임하고 중요한 비즈니스 로직을 도메인 객체가 행하게끔 한다.. 저번 코드 리뷰해주신거에 이어 신경써서 만든다는게 말씀해주신대로 하나에 모든 책임을 다 지게끔 만들었네요. 참고해보겠습니다!! 감사합니다

}

public static void racingCar(int number, int attempt) {
int[] positions = new int[number];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배열 말고 List를 사용해보면 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앞으로 개발하실때 특정 값을 저장해야 한다고 했을때 특별한 이유가 있지 않는 이상
자바 내장 Collection API에서 제공하는 자료구조를 사용하시는게 좋답니다 💪

int[] positions = new int[number];
Random random = new Random();

for (int i = 0; i < attempt; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 for문은 어떤행위를 하는걸까요? 🤔
처음 코드를 읽은 사람이 이 for문을 읽고 무엇을 하는것인지 한눈에 알 수 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for문을 별도의 메서드로 분리하고 메서드에 네이밍을 주면 어떨까요?

for (int i = 0; i < attempt; i++) {
            requestCarsToMove(number, positions, random);
            printRaceResult(positions);
}

private void requestCarsToMove(int number, int[] positions, Random random) {
        for (int j = 0; j < number; j++) {
            int forward = random.nextInt(10); // 0부터 9
            if (forward >= 4) { // 전진하는 조건
                positions[j] += 1;
            }
        }
 }

private void printRaceResult(int[] positions) {
        for (int pos : positions) {
            if (pos == 0) {
                System.out.print("ㅡ");
            }
            for (int k = 0; k < pos; k++) {
                System.out.print("ㅡ");
            }
            System.out.println();
        }
        System.out.println();
    }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어떤가요?

그냥 하나의 메서드 내부에 있는것과
메서드로 분리하고 메서드 이름으로 어떤 행위를 하고있는지 힌트를 주는것.

둘중에 어떤게 읽는 입장에서 더 읽기 쉬운 코드일까요~?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하지만 전반적으로 코드가 "객체지향적"이라기 보다는 "절차지향적"인 코드에 가까워보이네요 🤔
그래서 메서드로 나눠도 코드에 찝찝한 느낌이 남아있어요.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

한번 프로그램을 구현해서 "자동차 경주"라는 도메인에 대해 이해도가 올라갔으니,
이번에는 책임을 나눠서 객체들에게 할당하는것에 고민해보는게 어떨까요? 💪

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

행위에 대해 메서드 이름으로 힌트를 주는 것, 객체들에게 책임을 나누는 것 여러 번 되새겨서 제 것으로 만들어보겠습니다 감사합니다!!

Scanner scanner = new Scanner(System.in);
System.out.println("자동차 대수는 몇 대 인가요?");

int number = scanner.nextInt();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number는 무슨 숫자를 의미하는 걸까요?
처음 코드를 읽는 사람이 이 변수명만을 읽고 무슨 변수인지 알 수 있을까요? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"경주하는 자동차의 갯수" 라는 의미를 전달하는 변수명으로 바꾸는것이 좋을 것 같아요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조언 감사합니다!

int number = scanner.nextInt();

System.out.println("시도할 횟수는 몇 회인가요?");
int attempt = scanner.nextInt();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attempt는 시도의 정보를 담은 객체를 생각나게 하는 네이밍이네요~

numberOfAttempt 이런식으로 "시도하는 횟수"라는 의미를 전달하는 변수로 바꿔보는 것이 어떨까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

심플하게 attemptNo, attemptNum 정도도 괜찮겠네요 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의미를 전달하는 변수를 사용하는 것이 좋다는 조언 감사합니다!

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;

public class SetTest {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저번 요구사항을 반영해서 구체적인 테스트로 바꿔주셨군요 👍

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class StringTest {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다~ 💪


private ByteArrayOutputStream outputStream;

protected void systemIn(String input) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프로그래밍 요구사항을 지켜보아요!

프로그래밍 요구 사항

모든 로직에 단위 테스트를 구현한다.
단, UI(System.out, System.in) 로직은 제외

}

@Test
public void 자동차_수를_입력받는다() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

비즈니스 로직인 자동차 경주와 관련된 테스트는 보이지 않고 입출력에 관련된 테스트만 존재하는것 같네요 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아마도 이렇게 작성하신 이유는 "테스트를 작성하기 어려워서" 이겠죠?

그 이유는 현재 코드가 절차지향적으로 하나의 클래스안에 모든 로직이 혼재 되어있어서 그렇습니다.
테스트를 해야하는데 필요한 것들이 너무 많은 것이죠.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재의 코드를 테스트 하려면

  1. 입력을 받는다.
  2. 입력받은것을 Car 객체한테 넘겨준다.
  3. Car 객체가 내부적으로 비즈니스 로직을 수행 (내부에서 어떤일이 일어나는지 테스트 코드로 확인하기가 힘듬)
  4. Car 객체가 내부적으로 동작한 것의 결과물을 테스트(하지만 이것도 출력문이기 때문에 테스트하기가 어려움)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하지만 Car 객체의 책임을 나눠서 만들면 어떨까요?

StringCalcualtor를 만들었을때를 생각해봅시다.

StringCalcualtor는 파라미터로 매개변수로 값을 입력받아서 동작하기 때문에 입출력과 관련된것은 생각할필요가 없었죠?
-> Car 객체의 1,4의 어려움 해결

StringCalcualtor의 책임을 나눠서 만들었기 때문에 테스트하기가 간단함

값을 입력받아서 계산한다.
=> 1. 입력받은 값을 파싱한다.(StringParser) 2. 파싱한 값으로 계산한다.(StringCalculator)

이런식으로 나누어져 있었기때문에
파싱하는 책임을 테스트, 계산하는 책임을 테스트

이렇게 각각 테스트하면 됩니다. => 3의 어려움도 해결

이제 제가 하고싶은 이야기가 무엇인지 어느정도 감이 오실까요?

이것이 바로 객체지향과 절차지향의 차이랍니다. 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MVC 패턴을 알맞게 적용한 것인지는 모르겠으나 적용 중인데 책임들을 나눠주고보니 테스트코드를 작성하는데 어려움이 느껴지네요.. 자세한 조언 정말 감사합니다!!

import static study.racingcar.view.ResultView.printRaceResult;
import static study.racingcar.view.ResultView.requestCarsToMove;

public class CarController {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'CarController' 에서 로직을 직접 구현해서 사용하고 있네요!
현재의 구조에서는 Car객체와 CarController가 서로 바뀐듯처럼 보이네요 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CarController 라는 네이밍은 왜 나온것일까요?
게임기 컨트롤러 처럼 도메인 객체에게 어떠한 일을 하도록 시키는 녀석이 아닐까요?
현재의 구조에서는 Car 객체가 CarController에게 자동차 경주를 하도록 시키는 것으로 보이네요~

Copy link
Collaborator Author

@JoJeHuni JoJeHuni Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MVC 패턴이 컨트롤러 로직 실행 -> 비즈니스 로직 실행 후 결과를 받고 -> 결과 데이터를 모델에 저장 -> 컨트롤러에서 뷰 로직으로 권한을 넘겨준 뒤 뷰는 모델에서 데이터를 참조해 응답하는 것이라는걸 정리해놓고 활용할 때는 반대로 이행하고 있었네요.. 감사합니다!


public class CarController {

public static void racingCar(int carNum, int attemptNum) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

racingCar() 라는 메서드는 자동차 경주라고 해석할수도 있지만 사람에 따라 혼란스러울수도 있을것 같아요.
"자동차 경주를 시작한다."의 책임을 맡은 메서드니까 raceStart() 라고 지어보는건 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드에 책임과 연관된 네이밍을 지어보자는 조언 감사합니다!!

public class CarController {

public static void racingCar(int carNum, int attemptNum) {
List<Integer> positions = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"자동차의 위치"를 나타내는 positions 변수를 사용하셨네요~
그런데 뭔가 이상하게 느껴지지 않나요?
자동차 객체는 존재하지 않는데 "자동차의 위치"만 존재해서 로직을 수행하고 있네요 🤔

이 구조에서 자동차와 관련된 다른 기능이 추가되면 어떻게 될까요?
가령, 자동차의 색깔, 모양도 도메인 로직에 추가된다고 하면
colors, shapes를 추가해서 각각 사용해야하는 구조가 되지 않을까요~?

한번 고민해봅시다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차의 객체는 존재하지 않는데 위치만 존재한다는 말씀에 놀랐네요 고민해가며 수정해보겠습니다!!


public class Car {

public Car() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Car 객체가 왜 CarController를 호출해야 하는 것일까요?
RacingCarApplication에서 바로 CarController를 호출해도 되지 않을까요~?


public class InputView {

public static int numberOfCar() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numberOfCar() 라는 네이밍 보다는
메서드의 책임인 "경주하는 자동차의 갯수을 입력받는다"는 의미로 getCarNumbers() 라고 지어보면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 의견 감사합니다 !!!


public class ResultView {

public static void requestCarsToMove(int carNum, List<Integer> positions, Random random) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차에게 움직이라고 요청하는 도메인 로직이네요!
도메인으로 옮기는게 좋을것같아요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 받아보니 정말이네요 하면서는 순간 몰랐던 것 같습니다 감사합니다!

public static void requestCarsToMove(int carNum, List<Integer> positions, Random random) {
for (int j = 0; j < carNum; j++) {
int forward = random.nextInt(10); // 0부터 9
if (forward >= 4) { // 전진하는 조건
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if절은 따로 빼서 관리해보면 어떨까요?


public static void requestCarsToMove(int carNum, List<Integer> positions, Random random) {
for (int j = 0; j < carNum; j++) {
int forward = random.nextInt(10); // 0부터 9
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10이라는 숫자는 왜 나온것일까요?
이 코드를 처음보는 사람이 이것을 보고 맥락을 이해할 수 있을까요?
이렇게 단서없이 갑자기 튀어나오는 숫자를 매직 넘버라고 한답니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런경우 매직넘버는 별도의 상수로 관리해서 코드를 읽는 사람에게 단서를 주는것이 좋아요.

private static final int MAX_BOUNDARY_OF_RANDOM_NUMBER= 10;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매직넘버는 가급적 지양하자는 조언 감사합니다! 바로 수정하겠습니다


public static void printRaceResult(List<Integer> positions) {
for (int pos : positions) {
if (pos == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차의 위치가 0인데 출력을 하는 이유는 무엇인가요? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (int pos : positions) {
            for (int k = 0; k < pos; k++) {
                System.out.print("ㅡ");
            }
            System.out.println();
        }

이렇게 바꿔야 자동차의 위치에 맞게 출력하는 코드가 되겠네요.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 positions.stream()
                .forEach(pos -> {
                    System.out.println("ㅡ".repeat(pos));
                });

steam api를 사용하면 더 간단하게 사용할 수 있습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stream api에 대한 것은 몰랐네요.. 더 간단하게 할 수 있는 방법에 대한 조언도 감사합니다!!


public class CarTest {
@Test
public void 자동차_이동_로직을_확인한다() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트가 아직 제대로 작성되지 않았네요!
저번에 리뷰에서 말한것이지만 테스트를 작성했을때 어려웠다면
책임을 나누고 객체들 간의 관계를 정하는 작업을 더 해야한다는 신호가 아닐까요~?

조금씩 좋아지고 있습니다 👍

addCar(carNum, positions);
}

public static void raceStart(int carNum, int attemptNum) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Car 객체 내부에서 자동차 경주를 시작하고
자동차들을 생성하고 있네요!

뭔가 어색하다고 느껴지지 않나요?
raceStart()메서드는 컨트롤러로 보내고
컨트롤러의 raceStart()에서 자동차들을 생성하고
이동하라고 요청을 보내보는건 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨트롤러에서 자동차를 생성해서 요청을 보내면서 로직을 실행하고 모델에 저장한 뒤 뷰로 제어권을 넘겨주려고 수정하고 있습니다!! 감사합니다


for (int i = 0; i < attemptNum; i++) {
car.requestCarsToMove(carNum, positions);
printRaceResult(positions);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

같은 이유로 raceStart() 메서드가 Car 객체로 들어와서
도메인이 "알 필요가 없는" print 로직이 존재하네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇네요.. print 로직은 ResultView로 옮기겠습니다!

}
}

public static void requestCarsToMove(int carNum, List<Integer> positions) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서도 Car객체가 Car객체한테 이동하라고 요청하는 형태가 되었군요 🤔

}
}

private static void addCar(int carNum, List<Integer> positions) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매서드 마다 static 키워드가 덕지덕지 붙어있는데, static 키워드를 붙이신 이유는 무엇일까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static 사용할 때를 아직 잘 몰랐던 것 같습니다. 타입을 변경할 때, 특정 상수를 사용할 때 static을 사용하게끔 수정해보겠습니다!


private static final int POSITIONS_INDEX = 0;
@Test
public void 자동차_대수와_시도_횟수를_1로_정해준_뒤_이동_로직을_확인한다() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차의 대수와 시도 횟수가 꼭 1이여야 하는 이유가 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차의 입장에서는 횟수가 몇번인지, 자동차 갯수가 몇개인지를 알 필요가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞습니다 이동 로직을 확인하는데 자동차의 대수는 상관없으니 수정해보겠습니다!!

Car car = new Car(carNum, positions);
car.raceStart(carNum, attemptNum);

int carPosition = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carPosition 변수가 필요한 이유는 무엇인가요? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차 내부에서 여러대의 자동차를 생성하고 레이스를 실행하는 기형적인 형태를 가지게 되어서,
테스트 할때 자동차의 갯수와 횟수를 특정 숫자로 강제하는 형태가 되었네요.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차를 생성하고 레이스를 실행하는 책임을 다른 객체에게 위임하고
자동차는 위치, 이동할지 말지에 대한 책임만을 가지고 관리하게 만들어 보면 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런식으로 책임을 나누게 되면 번거롭게 carPosition 변수를 만들고 할 필요없이,
바로 Car 객체한테 현재의 위치를 알려달라고 요청하는 형태로 바꿀 수 있겠네요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동차 생성, 레이스 실행의 책임은 컨트롤러에게, 자동차는 이동하는 '기능'만 가지게끔 수정해보겠습니다. 조언 감사합니다

Copy link
Collaborator

@jonghyunhub jonghyunhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조금만 더 고민해봅시다!

return finalPositions;
}

private static List<Integer> addCar(int carNum) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addCar() 메서드 명만을 봤을때 Car 객체들을 생성해줄 것같은데 Integer들을 생성해주고 있네요 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Car 객체를 생성해준다는 것을 놓치고 있었네요 바로 수정하겠습니다. 감사합니다!!

}

public static List<Integer> raceStart(int carNum, int attemptNum) {
List<Integer> startLine = addCar(carNum);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

분명히 Car객체는 존재하는데도 불구하고
출발했을때의 위치, 경주가 끝났을때의 위치를 각각 만들어서 구현하신 이유가 무엇일까요? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 코드에서 raceStart 메서드의 반환값으로 assertEquals 를 사용하려고 만들었던 것 같습니다. 다시 생각해보겠습니다!

for (int i = 0; i < attemptNum; i++) {
List<Integer> racingCar = requestCarsToMove(carNum, startLine);
ResultView.printRaceResult(racingCar);
System.out.println("");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

출력은 ResultView에게 위임해보아요~


public static List<Integer> raceStart(int carNum, int attemptNum) {
List<Integer> startLine = addCar(carNum);
List<Integer> finalPositions = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

레이스가 시작하기전에 경주에 참여할 자동차 객체들을 생성하고
반복문을 통해서 자동차에게 앞으로 전진할지 말지를 요청해보는 식으로 만들어보면 어떨까요~?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raceStart 에서 addCar 메서드를 불러오는 것이 아닌 그 전에 미리 객체들을 생성하고 요청해보자는 조언 감사합니다! 수정하겠습니다!

}

private static List<Integer> addCar(int carNum) {
List<Integer> positions = new ArrayList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

position을 자동차 객체의 맴버변수로 바꿔서 로직을 만들어보면 어떨까요~?
자동차의 위치는 존재하는데 자동차는 존재하지 않는 이상한 상황이 벌어지고 있네요 😨

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer로 받는 것이 아닌 Car 객체의 ArrayList 를 만들어보겠습니다!


public static void requestCarsToMove(int carNum, List<Integer> positions) {
public static List<Integer> requestCarsToMove(int carNum, List<Integer> positions) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

열심히 만든 Car 객체를 자동차 경주에 참가시켜봅시다 💪

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

한 차례 더 수정을 거치는 중이라 곧 커밋하겠습니다 조언 항상 감사합니다!

int carPosition = 0;
public void 자동차_이동_로직을_확인한다() {
// given
Random random = new Random();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서도 자동차 로직을 테스트 하는데 자동차는 없고 자동차의 위치가 둥둥 떠다니고 있네요 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

바로 수정해보겠습니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants